Conversation
romain-intel
left a comment
There was a problem hiding this comment.
Comments to update PR.
2d2b74a to
853620c
Compare
Greptile SummaryThis PR makes the FlowSpec graph more flexible by introducing explicit Confidence Score: 4/5Safe to merge pending resolution of previously-flagged P1 threads; no new blocking issues found. All new findings in this pass are P2 style/quality issues. Previously-flagged P1 issues (orchestrator _graph_endpoints fallback, step_functions hardcoded keys, Argo end-event alias) are addressed in the PR diff. Score sits at the P1-ceiling of 4 reflecting caution from those prior threads. metaflow/lint.py (double-warning overlap); metaflow/util.py (overly broad callable check in to_pod); metaflow/client/core.py (_graph_endpoints fallback for orchestrated runs, already flagged in prior thread) Important Files Changed
Reviews (22): Last reviewed commit: "test: slim _graph_endpoints unit tests, ..." | Re-trigger Greptile |
583fc92 to
2902c34
Compare
romain-intel
left a comment
There was a problem hiding this comment.
minor comments with a quick review.
4ff334e to
23581b3
Compare
romain-intel
left a comment
There was a problem hiding this comment.
Minor nits. The workaround for no code will probably see evolution but it's not user facing (much). I'd just clarify a bit why it's there and we can merge.
8abb8fc to
accd71d
Compare
Instead of enforcing a step named "start" and another named "end" and a minimum of two steps, Metaflow now allows for single step flows with any names. The "start" and "end" properties are derived from the structure of the graph. We still require a single entry point and a single exit point but they can be one and the same and do not have to be named something specific.
Core fixes: - events.py: use end_task.parent.id instead of hardcoded run_obj["end"] - step_functions.py: use StartAt from definition JSON (both get_existing_deployment and get_execution) instead of hardcoded ["States"]["start"] - argo_workflows.py: _matching_conditional_join uses self.graph.end_step instead of hardcoded "end" fallback - graph.py: None guard in output_steps() raises clear ValueError - client/core.py: narrow _graph_endpoints exception caching to (KeyError, MetaflowNotFound); transient errors return uncached fallback - runtime.py: guard metadata registration with is_cloned check + try/except Cards: - card_cli.py: wrap graph_dict in payload with start_step/end_step metadata - basic.py: accept new payload format, pass through start/end - dag.svelte, step-wrapper.svelte, types.ts: dynamic start/end props Lint: - Improved check_basic_steps to distinguish "no start step" from "multiple @step(start=True) annotations" - New check_annotation_name_conflict: warns when @step(start=True) coexists with a step named "start" (and likewise for end) Tests: - Negative-path tests for malformed annotation patterns (8 cases) - Card rendering with custom endpoints - Trigger.from_runs() with custom terminal step - Step Functions StartAt lookup Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
FlowGraph._create_nodes walks dir(cls) by attribute name and stores nodes[element] = node, but DAGNode.name was derived from the AST def name. Those agreed only by convention. When a metaclass renames a function via __name__ (e.g. FunctionSpec synthesizing a step), AST parsing still reads the original def name, leaving nodes keyed by the attribute name while node.name reports the def name. FlowSpec._init_graph then does getattr(cls, node.name) and fails with AttributeError. Pass the discovered attribute name explicitly to DAGNode in the AST path, and have DAGNode prefer that over func_ast.name. Both paths (AST and the sourceless single-step fallback) now key on the same identifier. No behavior change for flows where def name and attribute name coincide, which is every ordinary flow.
Two fixes addressing PR review feedback on custom-named start/end steps: - argo_workflows_decorator.py: when the step emitting the auto-event is the graph's end step but has a custom name, also publish the event with the well-known ".end" suffix. @trigger_on_finish subscribers construct event names as "metaflow.<project>.<flow>.end" at deploy time and don't know the publisher's end step name, so without this dual emission, downstream flows silently miss triggers from custom-named end steps. - card_cli.py: revert the `graph` kwarg to the old shape (dict of step_name -> info) to avoid breaking third-party card modules that expect the pre-annotation format. Cards that need start/end metadata can opt-in by accepting a new `graph_info` kwarg — card_cli probes the constructor signature and only passes it when the card declares it. - basic.py: DefaultCard, DefaultCardJSON, and ErrorCard now accept `graph_info` (preferred) in addition to `graph` (backward compat).
The `func_ast=None`/`name`/`num_args` fallback in DAGNode, the `_create_sourceless_single_step_node` method, and the `_function_spec_step_name` branch in `_base_step_decorator` all exist to support an upcoming FunctionSpec feature (currently shipped as an out-of-tree extension). Without that context the code looks arbitrary — `_function_spec_step_name` is never set anywhere in the tree, and tolerating a missing AST seems like a design smell. Add a module-level explainer on DAGNode that lays out the FunctionSpec contract in one place, plus targeted inline pointers at each integration point (DAGNode.__init__, num_args init, _create_sourceless_single_step_node, _create_nodes except handler, and _base_step_decorator) that reference back to it. Also flag these hooks as potentially temporary — they may be removed if FunctionSpec is folded into core. No behavior change.
- events.py: Trigger.from_runs now emits both the step-name-specific
event ("metaflow.<flow>.<end_step>") and the well-known ".end" alias
for custom-named end steps. This mirrors the dual-emit in
argo_workflows_decorator.py so the Argo publish path and the
programmatic from_runs path stay symmetric -- downstream filters on
".end" keep matching from either source.
- argo_workflows_decorator.py: fix stale idempotency comment that was
stranded inside the new dual-emit loop. Clarify that retries
re-publish the same event(s) and that dedupe (if needed) happens on
the Argo Events side via the stable "id" payload.
- step_functions.py: replace the chained .get().get().get() in
get_execution with explicit guards that produce a readable error
("State machine X has no state named Y") rather than an opaque
AttributeError when a non-Metaflow state machine has an unexpected
shape.
- client/core.py: clarify the Run._graph_endpoints docstring to note
that every runtime path (native + orchestrators via the init command)
writes the _parameters metadata, so the single-tier lookup with a
literal fallback is sufficient.
Tests (13 new, 90 total passing):
- test_to_pod.py: covers primitives, containers, and the callable ->
__qualname__ serialization used by DAGNode.node_info.
- test_graph_endpoints_fallback.py: covers metadata hit, empty metadata,
partial metadata, caching, transient-error non-caching, and
MetaflowNotFound caching.
Replace direct imports of unittest.mock.MagicMock with the pytest-mock mocker fixture per team convention. The MagicMock-based helper becomes a make_run pytest fixture that takes mocker, so cleanup is handled per test. test_transient_error_not_cached uses mocker.MagicMock(side_effect=[...]) instead of a hand-rolled call-count closure. Add pytest-mock to devtools/requirements-devstack.txt so the fixture is available in tox -e unit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Cover the scenarios raised in PR review for @step(start=True, end=True) single-step flows: Config descriptors, stacked step decorators, and FlowMutators. Unit tests (test/unit/test_graph_structure.py): - Config descriptor is registered via _get_parameters. - @Retry + @resources stack correctly on the only step. - FlowMutator registers at class-definition time (pre_mutate only fires via the CLI layer, so execution is covered by the integration test). Integration tests (test/unit/graph_inference/, new flow files + fixtures): - Config-bearing single-step flow runs end-to-end; config value flows through to the end task's artifact. - Stacked @retry/@resources flow runs; _graph_info records both decorators on the only step. - FlowMutator-decorated flow runs; pre_mutate lands @Retry on the sole step. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pre-commit black hook collapsed the first new fixture's create_flow_fixture call onto a single line (fits within line length); the two longer ones stay multi-line as black reformatted them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
A flow with exactly one step is unambiguous: that step is both the entry and the terminal node, with or without an annotation. Make the ergonomic case work without forcing users to write @step(start=True, end=True) for the trivial single-step shape. Implementation: after the existing @step(start=...) / @step(end=...) resolution, when the graph has exactly one node, point start_step and end_step at it. Multi-step flows are unaffected, so the explicit annotation contract still holds for real DAGs. Drop the not-name.startswith("_") filter from _identify_start_end's _resolve closure and from lint.check_basic_steps. The filter was guarding against _parameters bleeding into the annotated-step list, but _parameters is a runtime-only synthetic step and never appears in FlowGraph.nodes. lint.check_step_names already rejects user step names that start with "_", so there is nothing left for the filter to exclude. Tests: new SingleStepBareFlow plus six tests mirroring the existing SingleStepFlow coverage (completes, graph_info, _parameters metadata, end_task, step iteration, parent/child empty). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The dag.svelte changes in 211618d (Fix start/end step regressions) added flow-without-start/end rendering support but were never bundled into main.js. CI does not rebuild the card UI, so the deployed cards have been rendering against stale bundled code. Re-run `npm run build` in metaflow/plugins/cards/ui/ to regenerate the bundle from the current Svelte source. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The defensive isinstance guards on graph and graph["steps"] were no-ops in practice: the legacy else branch already relies on graph being dict-like (uses `in graph`), so an isinstance gate that lets non-dicts fall through to that branch would just defer the error. The dict check on graph["steps"] guarded a phantom collision (an old-shape dict with steps named "steps", "start_step", and "end_step" simultaneously). Drop both, leaving the three sentinel-key membership tests that actually distinguish the new and legacy graph shapes. Switch the two graph.get() calls inside the if-branch to []-indexing since their keys are confirmed present. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pure test-file rename: where s iterates over Run/Steps and d iterates over decorator entries, use the descriptive names instead. No behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The dynamic single-step flow helper and its test in test_graph_structure.py were testing the FunctionSpec hook contract (sourceless DAGNode construction via _create_sourceless_single_step_node), not graph topology. Move them into test_sourceless_dag_node.py with a docstring that names the contract directly so a future contributor does not have to follow the chain helper -> graph.py module comment -> FunctionSpec backstory. No behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bundle the three card-related tests into test_card_dag.py: - test_transform_flow_graph_supports_explicit_endpoints - test_transform_flow_graph_keeps_legacy_start_end_detection - test_default_card_includes_custom_graph_endpoints All three target the cards' graph data layer (transform_flow_graph and the DAG component render path). Keeping them next to the graph-execution tests in test_graph_inference.py blurred file boundaries and meant test_graph_inference.py had to import DefaultCardJSON, transform_flow_graph, and a four-line importlib helper just for the one render test. To support the render test cleanly, hoist the FlowSpec class loader into conftest.py as _load_flow_class plus a custom_named_card_flow fixture. DefaultCardJSON.render() does getattr(self.flow, step_name) and inspect.getsource(...) to build the Task Code panel and crashes if flow is None, so the test still needs the actual class object; moving the loader into conftest gets the importlib dance out of the test files. No behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The happy-path coverage (custom-named-step flows producing the right
endpoints) lives in test/ux/core/test_basic.py, which runs real flows
via the Runner / scheduler deployer chain across local and scheduler
modes. Three of the unit tests in this file were just re-asserting that
contract without exercising the metadata layer end-to-end.
Drop:
- test_metadata_carries_endpoints (covered by test_custom_step_names)
- test_partial_metadata_fills_in_defaults (edge case, dict.get semantics)
- test_result_cached (cache write is implicitly verified by
test_metaflow_not_found_caches_fallback)
Replace the make_run factory fixture and the __class__ swap trick with
mocker.patch.object(Run, "__getitem__", ...) on a Run.__new__(Run)
instance. This is the pytest-mock idiom now codified in
CONTRIBUTING.md and matches how other unit tests will look going
forward.
Three tests remain, each pinning a non-obvious behavior:
- empty metadata falls back to ("start", "end")
- MetaflowNotFound (old run) caches the fallback
- transient exceptions do NOT cache (so a retry can succeed)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
0092a57 to
1e9f658
Compare
|
Let's go!!! |
PR Type
Summary
Makes the FlowSpec graph more flexible in three ways, with backward compatibility preserved throughout:
Explicit start/end annotations.
@stepnow acceptsstart=Trueandend=Truekwargs. Steps no longer have to be named "start"/"end" — a flow can use any name as long as one step is annotated as the start and one as the end. When no annotations are present, the graph falls back to discovering steps by the names "start" and "end" (old behavior).Single-step flows. A single step annotated
@step(start=True, end=True)is now valid; previously a FlowSpec required at least two steps.Per-node
node_infometadata.DAGNodegains anode_infodict that extensions can populate to attach arbitrary per-step metadata. Live references are accessible viaflow._graph; serialized values (viato_pod) flow into_graph_info. Callables are serialized as their qualified name.These are core-runtime changes because they touch
graph.py,lint.py,runtime.py,task.py, and every orchestrator plugin (Argo Workflows, Step Functions, Airflow) that reads the graph or emits events tied to the terminal step.Key changes
decorators.py@step(start=False, end=False, node_info=None)graph.pyDAGNode.is_start_step,is_end_step,node_info;_identify_start_enduses annotations with name fallback; attribute name (not ASTdefname) is authoritative; tolerates sourceless single-step methods (for out-of-tree FunctionSpec extension)lint.pycheck_basic_stepsdistinguishes "no start" from "multiple start annotations"; newcheck_start_end_degreevalidates in/out degrees; newcheck_annotation_name_conflictwarns when@step(start=True)coexists with a step named "start" (same for end)util.pyto_podserializes callables via__qualname__runtime.pystart_step/end_stepas metadata on the_parameterstasktask.py,flowspec.pyclient/core.pyRun._graph_endpointsreads endpoints from_parametersmetadata with_graph_infofallback for orchestrated runsplugins/argo/argo_workflows.py_matching_conditional_joinfalls back tograph.end_step; terminal-step detection through the DAGplugins/argo/argo_workflows_decorator.py.endsuffix so@trigger_on_finishsubscribers — which don't know the publisher's end step name at deploy time — keep receiving triggers from custom-named end stepsplugins/aws/step_functions/step_functions.pyStartAtfrom the definition JSON instead of hardcoded["States"]["start"]plugins/airflow/airflow.py,airflow_cli.pygraph.start_step/end_stepinstead of hardcoded namesplugins/cards/card_cli.pygraphkwarg keeps the old dict-of-steps shape for backward compatibility with third-party card modules; new opt-ingraph_infokwarg carries the full payload (steps + start/end). Card_cli probes the constructor signature and only passesgraph_infoto cards that accept itplugins/cards/card_modules/basic.pyDefaultCard,DefaultCardJSON,ErrorCardaccept bothgraphandgraph_info(preferred when provided)plugins/cards/ui/Tests
New and updated tests cover:
@step(start=True)/@step(end=True)annotation mechanics and backward-compat with name-based discoveryTrigger.from_runson flows with a custom-named terminal stepStartAtlookup from deployment JSONNon-Goals
StepSpec/FunctionSpecconstruct (single-step FlowSpec-like class withinit()+call()lifecycle) — hooks for it are present inDAGNodeand_base_step_decorator(see module comment ingraph.py), but the class itself is kept out-of-tree for now.FlowSpec.main()classmethod to route between multiple FlowSpec subclasses in one file) — deferred.AI Tool Usage
AI coding assistance was used during development. All generated code was reviewed and tested.